Open
Conversation
Contributor
Author
|
More test need to be written |
There was a problem hiding this comment.
Pull request overview
This PR adds unit tests for the work model functionality in the load balancing library and introduces a simplified Task constructor to facilitate test creation.
- Adds comprehensive unit tests for
WorkModelCalculatorfunctionality including work computation, memory usage, and incremental updates - Introduces a simplified
Task(TaskType id, LoadType load)constructor for easier task creation in tests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/work_model/test_work_model.cc | New test file with 4 test cases covering work computation, memory usage calculation, memory fit checking, and work update operations |
| src/vt-lb/model/Task.h | Adds simplified two-parameter constructor for creating Task instances with just ID and load |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -75,6 +75,10 @@ struct TaskMemory { | |||
|
|
|||
| struct Task { | |||
| Task() = default; | |||
There was a problem hiding this comment.
[nitpick] Consider adding documentation for this constructor to clarify its intended use case and the default values that will be used for uninitialized fields (home_, current_, migratable_, memory_, shared_blocks_). This would help other developers understand when to use this simplified constructor versus the full constructor.
Suggested change
| Task() = default; | |
| Task() = default; | |
| /** | |
| * @brief Simplified constructor for Task. | |
| * | |
| * Initializes a Task with the given id and load. | |
| * Other fields are set to their default values: | |
| * - home_ = invalid_node | |
| * - current_ = invalid_node | |
| * - migratable_ = true | |
| * - memory_ = default-constructed TaskMemory (all fields 0.0) | |
| * - shared_blocks_ = empty set | |
| * | |
| * Use this constructor when only id and load are known, and other fields can be defaulted. | |
| * For full control, use the more detailed constructor. | |
| */ |
lifflander
force-pushed
the
16-write-work-model-tests
branch
from
a4c6eac to
e2cee9f
Compare
lifflander
force-pushed
the
16-write-work-model-tests
branch
from
e2cee9f to
822e065
Compare
nlslatt
force-pushed
the
16-write-work-model-tests
branch
from
c841482 to
8f9d587
Compare
nlslatt
force-pushed
the
16-write-work-model-tests
branch
from
4698fc3 to
f7b46c5
Compare
nlslatt
force-pushed
the
16-write-work-model-tests
branch
from
3684e85 to
50e851e
Compare
nlslatt
force-pushed
the
16-write-work-model-tests
branch
from
b946c0a to
2e4c643
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16